More refined dependency management for different app types#70
More refined dependency management for different app types#70
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds error handling for missing pyproject.toml files in the uv sync operation and introduces basic regression tests for the tower-runtime crate. The main change skips the sync operation when no pyproject.toml is present rather than allowing uv to fail.
- Adds early detection and graceful handling of missing
pyproject.tomlfiles - Introduces comprehensive test coverage for LocalApp runtime with two example scenarios
- Updates Status enum to support equality comparisons needed for testing
Reviewed Changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tower-uv/src/lib.rs | Adds MissingPyprojectToml error variant and pre-sync validation |
| crates/tower-runtime/src/local.rs | Updates sync logic to gracefully handle missing pyproject.toml |
| crates/tower-runtime/src/lib.rs | Adds PartialEq trait to Status enum for test assertions |
| crates/tower-runtime/src/errors.rs | Maps new MissingPyprojectToml error to SpawnFailed |
| crates/tower-runtime/tests/local_test.rs | Adds comprehensive test suite with example apps |
| crates/tower-runtime/Cargo.toml | Adds config dev dependency for testing |
Comments suppressed due to low confidence (2)
crates/tower-runtime/tests/local_test.rs:71
- The error message 'App should be running' is incorrect for an assertion that checks if status equals Status::Exited. It should be 'App should be exited' or 'App should have exited'.
assert!(status == Status::Exited, "App should be running");
crates/tower-runtime/tests/local_test.rs:123
- The error message 'App should be running' is incorrect for an assertion that checks if status equals Status::Exited. It should be 'App should be exited' or 'App should have exited'.
assert!(status == Status::Exited, "App should be running");
| tower_uv::Error::NotFound(_) => Error::SpawnFailed, | ||
| tower_uv::Error::PermissionDenied(_) => Error::SpawnFailed, | ||
| tower_uv::Error::Other(_) => Error::SpawnFailed, | ||
| tower_uv::Error::MissingPyprojectToml => Error::SpawnFailed, |
There was a problem hiding this comment.
The indentation is inconsistent with the surrounding code. The line should be indented to align with the other match arms above it.
| tower_uv::Error::MissingPyprojectToml => Error::SpawnFailed, | |
| tower_uv::Error::MissingPyprojectToml => Error::SpawnFailed, |
There was a problem hiding this comment.
I don't understand this one? Maybe there's something messed up in my local enviro config.
There was a problem hiding this comment.
Seems to be to me? Thought maybe it was some hidden whitespace issue perhaps.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
bradhe
changed the title
crates/tower-runtime/src/lib.rs
Outdated
| let (sender, receiver) = unbounded_channel::<Output>(); | ||
|
|
||
| let output_sender = Arc::new(Mutex::new(sender)); | ||
| let output_receiver = Arc::new(Mutex::new(receiver)); | ||
| (output_sender, output_receiver) | ||
| (sender, receiver) |
There was a problem hiding this comment.
- You can elide the let binding completely now, so just calling
unbounded_channel - If that's the case, you could possibly elide this function completely?
- Is there a reason we don't use a bounded channel? Unbounded channels use a linkedlist. I presume if we have a large enough bounded channel we're unlikely to ever get backpressure, and it may improve latency due to being friendlier to cpu cache?
There was a problem hiding this comment.
You're right about eliding the let binding, I think we can do that. The only purpose of this function is to manage the creation of our internal type, OutputSender and OutputReceiver. This kind of "hides" the fact that we use tokio under the hood.
Anyway, we might be able to push all this on to the user. I'd say lets experiment with that a bit later on.
There was a problem hiding this comment.
Actually you were right about eliding this whole thing away. Nice.
crates/tower-uv/src/lib.rs
Outdated
| match test_uv_path(&self.uv_path).await { | ||
| Ok(_) => true, | ||
| Err(_) => false, | ||
| } |
There was a problem hiding this comment.
untested, but can probably do this?
| match test_uv_path(&self.uv_path).await { | |
| Ok(_) => true, | |
| Err(_) => false, | |
| } | |
| (&self.uv_path).await.is_ok() |
Co-authored-by: Ben Lovell <ben.j.lovell@gmail.com>
socksy
left a comment
socksy
left a comment
There was a problem hiding this comment.
LGTM, and the core logic doesn't seem to break when I throw some projects with requirements.txt at it etc? not 100% sure if I effectively tested it though 😅
This PR does a few different things.
uv syncfor setup.It also adds some test coverage for running different types of apps via
tower-runtimeto ensure the behavior doesn't change over time.